Skip to content

Conversation

@aldevv
Copy link
Contributor

@aldevv aldevv commented Apr 14, 2025

No description provided.

@aldevv aldevv mentioned this pull request Apr 14, 2025

for _, team := range teams {
fullTeam, _, err := o.client.Teams.GetTeamByID(ctx, orgID, team.GetID())
fullTeam, _, err := o.client.Teams.GetTeamBySlug(ctx, fmt.Sprintf("%v", orgID), fmt.Sprintf("%v", team.GetID()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change caused syncs to fail for me with a 404.

{"level":"error","ts":1744830589.5247319,"caller":"ugrpc/logging.go:35","msg":"finished unary call with code Unknown","grpc.start_time":"2025-04-16T12:09:49-07:00","grpc.service":"c1.connector.v2.ResourcesService","grpc.method":"ListResources","peer.address":"127.0.0.1:52850","error":"error: listing resources failed: GET https://api.github.com/orgs/82982809/teams/4820527: 404 Not Found []","grpc.code":"Unknown","grpc.time_ms":244.131}

I think you want

fullTeam, _, err := o.client.Teams.GetTeamBySlug(ctx, orgName, team.GetSlug())

The same goes for the other API calls that get a team.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right, fixed

@ggreer
Copy link
Contributor

ggreer commented Apr 16, 2025

It would also be nice if the CI tests synced secrets so that this code was tested.

@abovee
Copy link

abovee commented Apr 28, 2025

@btipling do you know if we're syncing class github tokens as well? or just the new fine grained access tokens?

@aldevv
Copy link
Contributor Author

aldevv commented May 19, 2025

@btipling do you know if we're syncing class github tokens as well? or just the new fine grained access tokens?

only fine grained access tokens

Copy link
Contributor

@btipling btipling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comment

type apiTokenResourceType struct {
resourceType *v2.ResourceType
client *github.Client
hasSAMLEnabled *bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems unused, seems to be copy pasted from user.go

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

@btipling btipling force-pushed the update/sync-secrets-new branch from 1c94ed3 to 74ff2eb Compare May 19, 2025 22:52
Copy link
Contributor

@btipling btipling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually after rebasing I see it is using a slug as an id. I am not sure my rebase is correct, also we can't use a slug as an id, we can only use the id as an id. The slug is not an identifier we should not use it as an identifier, if the id not enough information we need to add more information to the id. Although, we should also not update ids as they will churn stuff. I hope we don't need the slug and everything is in the id.

@aldevv
Copy link
Contributor Author

aldevv commented May 19, 2025

Actually after rebasing I see it is using a slug as an id. I am not sure my rebase is correct, also we can't use a slug as an id, we can only use the id as an id. The slug is not an identifier we should not use it as an identifier, if the id not enough information we need to add more information to the id. Although, we should also not update ids as they will churn stuff. I hope we don't need the slug and everything is in the id.

only reason I made the change to slug is because the method GetTeamByID is deprecated, rebase appears to have broken teams.go so gonna push a fix

@btipling btipling force-pushed the update/sync-secrets-new branch 2 times, most recently from 0714ec4 to 8fac5be Compare May 20, 2025 22:58
@btipling btipling dismissed their stale review May 20, 2025 23:10

dismissing review

update readme

fix config file

fix github deprecated calls

fix github deprecated calls 2

fix lint warnings

refactor resource type

add secrets resource

fix github deprecated calls

fix getUserBySlug using id instead of name, remove dead code

fix lint

fix readme

update golangci-lint

use v8
@btipling btipling force-pushed the update/sync-secrets-new branch from 8fac5be to 32500bb Compare May 21, 2025 16:48
@btipling btipling merged commit 0538745 into main May 21, 2025
2 checks passed
@btipling btipling deleted the update/sync-secrets-new branch May 21, 2025 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants